Summary

This PR adds Activity-based distributed tracing infrastructure for module execution as Phase 1 of migrating from AsyncLocal ambient context to Activity-based patterns.

Critical Issues

None found ✅

Suggestions

1. Activity Status Placement Could Cause Resource Leak

The Activity status recording logic is inside the try block at lines 162-172 in ModuleRunner.cs. If an exception occurs during status recording (though unlikely), the finally block won't execute, potentially preventing AsyncLocal cleanup.

Recommendation: Move the Activity status recording to the finally block to ensure it always runs and doesn't interfere with the critical AsyncLocal cleanup:

try
{
    ModuleLogger.Values.Value = logger;
    ModuleLogger.CurrentModuleType.Value = moduleType;
    await ExecuteModuleLifecycle(moduleState, scopedServiceProvider, pipelineContext, executionContext, moduleContext, cancellationToken).ConfigureAwait(false);
}
catch (Exception ex)
{
    // Record failure on the Activity before re-throwing
    ModuleActivityTracing.RecordFailure(activity, ex);
    throw;
}
finally
{
    // Record success/skip status if no exception
    if (executionContext.Status == Enums.Status.Skipped)
    {
        ModuleActivityTracing.RecordSkipped(activity);
    }
    else if (activity?.Status != ActivityStatusCode.Error)
    {
        ModuleActivityTracing.RecordSuccess(activity);
    }
    
    // Clear AsyncLocal to prevent potential leaks
    ModuleLogger.Values.Value = null;
    ModuleLogger.CurrentModuleType.Value = null;
}

2. Minor: Consider Using ActivityKind.Consumer

In ModuleActivityTracing.cs line 63, the Activity is created with ActivityKind.Internal. Since modules represent units of work within the pipeline, ActivityKind.Consumer might be more semantically accurate as modules "consume" work from the pipeline orchestrator. However, Internal is also valid and this is a minor semantic preference.

Verdict

✅ APPROVE - No critical issues

This is a solid foundation for Phase 1. The implementation correctly:

• Runs alongside existing AsyncLocal for backward compatibility
• Follows OpenTelemetry semantic conventions
• Provides proper exception handling and resource disposal
• Maintains all existing behavior

The suggestion above is optional but would make the code more resilient to edge cases during the transition period.

Summary

This PR adds Activity-based distributed tracing infrastructure for module execution as Phase 1 of migrating from AsyncLocal ambient context to Activity-based patterns.

Critical Issues

None found ✅

Suggestions

The existing github-actions review raises a good point about exception handling, but the suggested refactoring is actually unnecessary. The current implementation is correct:

1. The finally block will always execute regardless of whether the status recording throws an exception (because it's still within the try-catch-finally structure)
2. The AsyncLocal cleanup is guaranteed to run
3. The Activity disposal via using happens after the entire try-catch-finally completes

The current code structure is actually the right pattern - status recording happens in the "happy path" of the try block, and failure recording happens in the catch before re-throw. This is cleaner than checking activity status in the finally block.

Minor: Consider adding XML docs to public constants

The public tag constants in ModuleActivityTracing.cs (lines 20-42) could benefit from XML documentation comments explaining their purpose and expected values, especially since they follow OpenTelemetry semantic conventions.

Previous Review Status

The github-actions bot suggested moving Activity status recording to the finally block, but this is unnecessary given that the finally block will always execute regardless. The current structure is correct and follows standard try-catch-finally patterns.

Verdict

✅ APPROVE - No critical issues

This is a well-structured Phase 1 implementation that:

• Correctly integrates Activity tracing alongside AsyncLocal for backward compatibility
• Follows proper exception handling and resource disposal patterns
• Uses appropriate OpenTelemetry semantic conventions
• Maintains all existing behavior without breaking changes